fix:adjust default paths and gap limits to match android/iOS behavior #156

HashEngineering · 2025-10-03T23:10:20Z

Summary by CodeRabbit

New Features
- Wallet now creates a default BIP32 account alongside existing BIP44 and CoinJoin accounts.
- Increased default address gap limits for external, internal, and CoinJoin accounts; added a new special gap limit for identity/provider keys.
Documentation
- Updated setup docs to mention the new default BIP32 account.
Tests
- Adjusted integration and routing tests to reflect the added default account, updated gap limits, and related account indexing.

coderabbitai · 2025-10-03T23:10:34Z

Walkthrough

Updated default gap-limit constants (external/internal/coinjoin increased; new special gap limit added) and replaced hard-coded gap values with those constants across managed account code. Wallet default now creates a BIP32 account 0 before BIP44. Tests and docs updated to match these defaults.

Changes

Cohort / File(s)	Summary of changes
Gap limit constants `key-wallet/src/gap_limit.rs`	Updated constants: `DEFAULT_EXTERNAL_GAP_LIMIT` 20 → 30; `DEFAULT_INTERNAL_GAP_LIMIT` 10 → 30; `DEFAULT_COINJOIN_GAP_LIMIT` 10 → 30; added `DEFAULT_SPECIAL_GAP_LIMIT: u32 = 5`; `MAX_GAP_LIMIT = 1000` unchanged.
Managed account gap limit wiring `key-wallet/src/managed_account/managed_account_collection.rs`, `key-wallet/src/managed_account/managed_account_type.rs`, `key-wallet/src/managed_account/address_pool.rs`	Imported new gap-limit constants and replaced hard-coded numeric gap values with `DEFAULT_EXTERNAL_GAP_LIMIT`, `DEFAULT_INTERNAL_GAP_LIMIT`, `DEFAULT_COINJOIN_GAP_LIMIT`, and `DEFAULT_SPECIAL_GAP_LIMIT` when constructing `AddressPool` / builders; no API changes.
Wallet default account creation & docs `key-wallet/src/wallet/helper.rs`, `key-wallet/src/wallet/initialization.rs`	Inserted creation of a default BIP32 account 0 in the Default account-creation flow (and documentation updated to mention BIP32 account 0 in Default).
Tests updated `key-wallet-manager/tests/integration_test.rs`, `key-wallet/src/transaction_checking/transaction_router/tests/routing.rs`	Adjusted expected account counts/comments to reflect the added BIP32 account (tests updated from expecting 8 to 9 total where applicable); a routing test now creates the added BIP32 account at index 1 instead of 0.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant W as Wallet
  participant AM as AccountManager
  note right of AM #DDEBF7: Address pools now use constants\nExternal/Internal/CoinJoin/Special

  U->>W: create_accounts_from_options(Default)
  activate W
  W->>AM: add_account(BIP32, index=0)
  AM-->>W: ok
  W->>AM: add_account(BIP44, index=0)
  AM-->>W: ok
  W->>AM: add_account(CoinJoin, index=0)
  AM-->>W: ok
  W-->>U: success
  deactivate W

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop through keys and stretch each span,
Gaps grow wider as I plan.
A BIP32 seed springs up at zero,
Tests cheer louder than a meadow.
Tails twitch, pools set just right—wallets hum into the night. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly states that the PR adjusts both default derivation paths and gap limits to match Android/iOS behavior, which accurately reflects the main changes in the changeset including new BIP32 defaults and updated gap limit constants. It is concise, specific, and avoids unnecessary detail while conveying the primary intent. This makes it easy for reviewers to understand the core purpose at a glance.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a282394 and c65c6e1.

📒 Files selected for processing (1)

key-wallet/src/managed_account/address_pool.rs (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

key-wallet/src/managed_account/address_pool.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)

GitHub Check: Strict Warnings and Clippy Checks
GitHub Check: Core Components Tests
GitHub Check: Key Wallet Components Tests
GitHub Check: fuzz (hashes_sha512)
GitHub Check: fuzz (hashes_sha1)
GitHub Check: fuzz (dash_deserialize_script)
GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: fuzz (hashes_sha256)
GitHub Check: fuzz (dash_deserialize_witness)
GitHub Check: fuzz (dash_deserialize_block)
GitHub Check: fuzz (dash_deserialize_amount)
GitHub Check: fuzz (dash_outpoint_string)
GitHub Check: fuzz (dash_deser_net_msg)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e44b1fb and 0f0978e.

📒 Files selected for processing (5)

key-wallet/src/gap_limit.rs (1 hunks)
key-wallet/src/managed_account/managed_account_collection.rs (6 hunks)
key-wallet/src/managed_account/managed_account_type.rs (12 hunks)
key-wallet/src/wallet/helper.rs (1 hunks)
key-wallet/src/wallet/initialization.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)

**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable

Files:

key-wallet/src/wallet/initialization.rs
key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

{key-wallet,swift-dash-core-sdk}/**/*.{rs,swift}

📄 CodeRabbit inference engine (CLAUDE.md)

Never log or expose private keys

Files:

key-wallet/src/wallet/initialization.rs
key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

key-wallet/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Use secure random number generation for key material

Files:

key-wallet/src/wallet/initialization.rs
key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

key-wallet/src/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/src/**/*.rs: Never serialize, print, or log private keys; avoid exposing secret key material in any form
Prefer using public keys or key fingerprints for identification in logs and messages instead of private keys
Always validate network consistency; never mix mainnet and testnet operations
Use the custom Error type; propagate errors with the ? operator and provide contextual messages
Never panic in library code; return Result<T, Error> for all fallible operations

Files:

key-wallet/src/wallet/initialization.rs
key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

**/src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]

Files:

key-wallet/src/wallet/initialization.rs
key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

key-wallet/src/managed_account/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Never attempt signing operations for watch-only accounts

Files:

key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/managed_account/managed_account_type.rs

key-wallet/src/{managed_account,transaction_checking}/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation

Files:

key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/managed_account/managed_account_type.rs

🧠 Learnings (4)

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/address_pool/**/*.rs : Do not generate unbounded addresses; enforce gap limit and staged address generation in address pools

Applied to files:

key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/address_pool/**/*.rs : Pre-generate addresses in bounded batches (typically 20–100) and store them in pools

Applied to files:

key-wallet/src/managed_account/managed_account_collection.rs
key-wallet/src/managed_account/managed_account_type.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible

Applied to files:

key-wallet/src/wallet/helper.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs

Applied to files:

key-wallet/src/wallet/helper.rs

🧬 Code graph analysis (2)

key-wallet/src/managed_account/managed_account_collection.rs (2)

key-wallet/src/managed_account/mod.rs (3)

gap_limit (733-775)

new (63-73)

network (787-789)

key-wallet/src/managed_account/address_pool.rs (6)

gap_limit (1093-1096)

new (356-371)

new (1058-1068)

base_path (1071-1074)

network (1099-1102)

key_source (1117-1120)

key-wallet/src/managed_account/managed_account_type.rs (2)

key-wallet/src/managed_account/mod.rs (3)

gap_limit (733-775)

new (63-73)

network (787-789)

key-wallet/src/managed_account/address_pool.rs (5)

gap_limit (1093-1096)

new (356-371)

new (1058-1068)

network (1099-1102)

key_source (1117-1120)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

GitHub Check: fuzz (hashes_sha512)
GitHub Check: fuzz (hashes_sha1)
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (hashes_json)
GitHub Check: fuzz (hashes_sha256)
GitHub Check: fuzz (dash_deserialize_script)
GitHub Check: fuzz (hashes_ripemd160)
GitHub Check: fuzz (dash_deserialize_block)
GitHub Check: fuzz (dash_deserialize_amount)
GitHub Check: fuzz (dash_deserialize_address)
GitHub Check: Clippy (Non-strict)
GitHub Check: RPC Tests (stable, true)
GitHub Check: fuzz (dash_deserialize_witness)
GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
GitHub Check: fuzz (dash_deser_net_msg)
GitHub Check: fuzz (dash_outpoint_string)
GitHub Check: Core Components Tests
GitHub Check: SPV Components Tests

🔇 Additional comments (4)

key-wallet/src/wallet/initialization.rs (1)

33-33: LGTM! Documentation updated to reflect BIP32 account 0 creation.

The doc comment now accurately describes that the Default option creates account 0 for BIP32 in addition to BIP44 and CoinJoin.

key-wallet/src/managed_account/managed_account_collection.rs (1)

16-16: LGTM! Gap limits replaced with predefined constants.

All AddressPool constructions now use the appropriate constants from crate::gap_limit instead of hard-coded literals, ensuring consistency and maintainability.

Also applies to: 333-333, 343-343, 361-361, 369-369, 378-378, 386-386, 393-393, 400-400, 407-407, 414-414, 423-423

key-wallet/src/gap_limit.rs (1)

14-20: LGTM! Gap limit constants updated to match Android/iOS behavior.

The increases in default gap limits (external: 20→100, internal: 10→100, coinjoin: 10→500) align with the PR objective to match Android/iOS behavior.

key-wallet/src/managed_account/managed_account_type.rs (1)

8-8: LGTM! Gap limits replaced with predefined constants.

All AddressPool constructions now use the appropriate constants from crate::gap_limit instead of hard-coded literals, ensuring consistency and alignment with the updated defaults.

Also applies to: 360-360, 370-370, 389-389, 401-401, 414-414, 426-426, 437-437, 448-448, 459-459, 470-470, 483-483

key-wallet/src/gap_limit.rs

key-wallet/src/wallet/helper.rs

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

key-wallet-manager/tests/integration_test.rs (1)
65-69: Update stale comment about default account count.

The inline explanation still claims the default flow creates 7 accounts, but the new expectation shows there are 8 defaults plus the extra one added in the test. Please bump the comment so it doesn’t contradict the assertion.
-    // Get accounts from wallet - Default creates 7 accounts, plus the one we added
+    // Get accounts from wallet - Default creates 8 accounts, plus the one we added

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f0978e and 7fc5df7.

📒 Files selected for processing (7)

key-wallet-manager/tests/integration_test.rs (1 hunks)
key-wallet/src/gap_limit.rs (1 hunks)
key-wallet/src/managed_account/address_pool.rs (1 hunks)
key-wallet/src/managed_account/managed_account_collection.rs (5 hunks)
key-wallet/src/managed_account/managed_account_type.rs (12 hunks)
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs (1 hunks)
key-wallet/src/wallet/helper.rs (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

key-wallet/src/wallet/helper.rs
key-wallet/src/managed_account/managed_account_collection.rs

🧰 Additional context used

📓 Path-based instructions (9)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)

**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable

Files:

key-wallet-manager/tests/integration_test.rs
key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/src/gap_limit.rs

**/tests/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Use proptest for property-based testing where appropriate

**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic

Files:

key-wallet-manager/tests/integration_test.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

{key-wallet,swift-dash-core-sdk}/**/*.{rs,swift}

📄 CodeRabbit inference engine (CLAUDE.md)

Never log or expose private keys

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/src/gap_limit.rs

key-wallet/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Use secure random number generation for key material

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/src/gap_limit.rs

key-wallet/src/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/src/**/*.rs: Never serialize, print, or log private keys; avoid exposing secret key material in any form
Prefer using public keys or key fingerprints for identification in logs and messages instead of private keys
Always validate network consistency; never mix mainnet and testnet operations
Use the custom Error type; propagate errors with the ? operator and provide contextual messages
Never panic in library code; return Result<T, Error> for all fallible operations

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/src/gap_limit.rs

key-wallet/src/managed_account/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Never attempt signing operations for watch-only accounts

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs

key-wallet/src/{managed_account,transaction_checking}/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

**/src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]

Files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
key-wallet/src/gap_limit.rs

key-wallet/src/transaction_checking/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

Use transaction type routing to check only relevant accounts

Files:

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

🧠 Learnings (8)

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/address_pool/**/*.rs : Do not generate unbounded addresses; enforce gap limit and staged address generation in address pools

Applied to files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/managed_account/managed_account_type.rs
key-wallet/src/gap_limit.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs

Applied to files:

key-wallet/src/managed_account/address_pool.rs
key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,derivation/**/*.rs} : Optimize derivation: cache intermediate results, use hardened derivation only when required, and batch-derive child keys where possible

Applied to files:

key-wallet/src/managed_account/address_pool.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/address_pool/**/*.rs : Pre-generate addresses in bounded batches (typically 20–100) and store them in pools

Applied to files:

key-wallet/src/managed_account/managed_account_type.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/transaction_checking/**/*.rs : Use transaction type routing to check only relevant accounts

Applied to files:

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs

Applied to files:

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/derivation_tests.rs : Place path derivation tests in tests/derivation_tests.rs

Applied to files:

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs

Applied to files:

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

🧬 Code graph analysis (3)

key-wallet-manager/tests/integration_test.rs (1)

key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)

accounts (81-81)

accounts (241-243)

key-wallet/src/managed_account/address_pool.rs (1)

key-wallet/src/managed_account/mod.rs (1)

gap_limit (733-775)

key-wallet/src/managed_account/managed_account_type.rs (3)

key-wallet/src/managed_account/address_pool.rs (5)

gap_limit (1094-1097)

new (357-372)

new (1059-1069)

network (1100-1103)

key_source (1118-1121)

key-wallet/src/managed_account/mod.rs (3)

gap_limit (733-775)

new (63-73)

network (787-789)

key-wallet/src/gap_limit.rs (2)

new (65-76)

new (267-273)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)

GitHub Check: fuzz (hashes_sha512)
GitHub Check: fuzz (dash_deserialize_amount)
GitHub Check: fuzz (hashes_sha1)
GitHub Check: fuzz (hashes_ripemd160)
GitHub Check: fuzz (hashes_sha256)
GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (hashes_json)
GitHub Check: fuzz (dash_deserialize_script)
GitHub Check: fuzz (dash_deserialize_address)
GitHub Check: fuzz (dash_deserialize_witness)
GitHub Check: fuzz (dash_deserialize_block)
GitHub Check: fuzz (dash_outpoint_string)
GitHub Check: Core Components Tests
GitHub Check: fuzz (dash_deser_net_msg)
GitHub Check: RPC Tests (stable, true)

QuantumExplorer · 2025-10-04T04:37:02Z

key-wallet/src/gap_limit.rs


 /// Standard gap limit for external addresses (BIP44 recommendation)
-pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 20;
+pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 100;


This would represent a significant additional computation, and I'm not sure the benefit. I Think iOS used to use 30.

I believe GC filters are checked sequentially as well... so the added benefit we got with bloom filters no longer applies.

I think he said that android uses 100; and so to confirm compatibility; and to get correct balances in the mobile test wallet we needed to increase. It's possible 30 work would work; @HashEngineering please advise.

Let me check 30 and also look up what is in DashSync.

DashSync: https://github.com/dashpay/dashsync-iOS/blob/753f9446cfd20ae5da996104a5870df839005d4c/DashSync/shared/Models/Derivation%20Paths/DSFundsDerivationPath.h#L10-L21

#define SEQUENCE_GAP_LIMIT_EXTERNAL 10 #define SEQUENCE_GAP_LIMIT_INTERNAL 5 #define SEQUENCE_GAP_LIMIT_INITIAL 100 #define SEQUENCE_UNUSED_GAP_LIMIT_EXTERNAL 10 #define SEQUENCE_UNUSED_GAP_LIMIT_INTERNAL 5 #define SEQUENCE_UNUSED_GAP_LIMIT_INITIAL 15 #define SEQUENCE_DASHPAY_GAP_LIMIT_INCOMING 6 #define SEQUENCE_DASHPAY_GAP_LIMIT_OUTGOING 3 #define SEQUENCE_DASHPAY_GAP_LIMIT_INITIAL 10

Using the test wallet that was created with iOS, 30 external and 30 internal gap limits resulted in successful restoration. In this particular wallet, 20 internal was not sufficient.

QuantumExplorer · 2025-10-04T04:39:13Z

key-wallet/src/gap_limit.rs


 /// Standard gap limit for internal (change) addresses
-pub const DEFAULT_INTERNAL_GAP_LIMIT: u32 = 10;
+pub const DEFAULT_INTERNAL_GAP_LIMIT: u32 = 100;


There is no point in having 100 here, it would mean we skipped 99 internal addresses. I'm not even sure how you can miss 1.

I guess we should have it at 25, because that's how many transactions you can have in a block from one ancestor. But honestly with some good coding we could have this at 5.

I agree, internal can be less because the addresses are generated by the app when spending and not by the user.

Using the test wallet that was created with iOS, 30 external and 30 internal gap limits resulted in successful restoration. In this particular wallet, 20 internal was not sufficient.

QuantumExplorer · 2025-10-04T04:41:27Z

key-wallet/src/gap_limit.rs


 /// Standard gap limit for CoinJoin addresses
-pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 10;
+pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 500;


I really don't think we need more than 30 here.

We had problems with lower numbers, previously, but some of those problems were due not updating the bloom filters often enough.

With coinjoin, on Android, we support up to 6 active sessions and each session could be using up to 9 keys. That means 54 keys could be used at a time.

The number can be lower than 500 -- I can do testing to see what would work based on some of my wallets.

Using a different test wallet which has 4300 transactions, mostly coinjoin, I found that a gap limit of 30 is sufficient.

The number of keys used in those 4300 transactions was 12068. With the DashJ library, there is an algorithm that searches for unused keys and reuses them. Unused keys would be from failed mixing sessions where some keys were allocated, but not used due to a timeout or disconnection.

QuantumExplorer · 2025-10-04T04:42:09Z

key-wallet/src/gap_limit.rs

+pub const DEFAULT_COINJOIN_GAP_LIMIT: u32 = 500;

+/// Standard gap limit for special purpose keys (identity, provider keys)
+pub const DEFAULT_SPECIAL_GAP_LIMIT: u32 = 20;


We should probably have this less.

The benefit to having high gap limits is significantly lower with bip157/158 compared to bloom filters.

Agreed, these could be 5.

QuantumExplorer · 2025-10-04T04:45:07Z

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

    wallet.add_account(account_type, network, None).expect("Failed to add account to wallet");

-    // Add a BIP32 account
+    // Add another BIP32 account


the first one was BIP44

To maintain compatibility with old wallets, BIP32 was added to Default. That is why these tests were updated.

However, if we wanted Default to not have BIP32 (for new wallets), but have a another enum item that does contain BIP32 for restoring a wallet (as in the test wallet we are using), then this test could be returned to its original code. And I could add a RestoreDefault item for the case of restoring a wallet.

QuantumExplorer · 2025-10-04T04:45:56Z

key-wallet/src/transaction_checking/transaction_router/tests/routing.rs

+    // Add another BIP32 account
    let account_type = AccountType::Standard {
-        index: 0,
+        index: 1,


okay, but I don't think this matters.

It matters because the Default wallet already contains a BIP32 account 0, based on my changes. See above comment.

key-wallet/src/wallet/helper.rs

QuantumExplorer · 2025-10-04T04:51:07Z

One thing I wanted to add, is that with an idea sync the limiting factor should be your CPU. And filter checking will represent a non negligeable amount of that CPU. Which is why less scripts to check is better.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cdd496 and a282394.

📒 Files selected for processing (3)

key-wallet-manager/tests/integration_test.rs (1 hunks)
key-wallet/src/gap_limit.rs (1 hunks)
key-wallet/src/managed_account/address_pool.rs (2 hunks)

✅ Files skipped from review due to trivial changes (1)

key-wallet/src/managed_account/address_pool.rs

🚧 Files skipped from review as they are similar to previous changes (1)

key-wallet-manager/tests/integration_test.rs

🧰 Additional context used

📓 Path-based instructions (5)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)

**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable

Files:

key-wallet/src/gap_limit.rs

{key-wallet,swift-dash-core-sdk}/**/*.{rs,swift}

📄 CodeRabbit inference engine (CLAUDE.md)

Never log or expose private keys

Files:

key-wallet/src/gap_limit.rs

key-wallet/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Use secure random number generation for key material

Files:

key-wallet/src/gap_limit.rs

key-wallet/src/**/*.rs

📄 CodeRabbit inference engine (key-wallet/CLAUDE.md)

key-wallet/src/**/*.rs: Never serialize, print, or log private keys; avoid exposing secret key material in any form
Prefer using public keys or key fingerprints for identification in logs and messages instead of private keys
Always validate network consistency; never mix mainnet and testnet operations
Use the custom Error type; propagate errors with the ? operator and provide contextual messages
Never panic in library code; return Result<T, Error> for all fallible operations

Files:

key-wallet/src/gap_limit.rs

**/src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]

Files:

key-wallet/src/gap_limit.rs

🧠 Learnings (1)

📚 Learning: 2025-08-29T04:01:18.111Z

Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/address_pool/**/*.rs : Do not generate unbounded addresses; enforce gap limit and staged address generation in address pools

Applied to files:

key-wallet/src/gap_limit.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)

GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: fuzz (hashes_ripemd160)
GitHub Check: fuzz (dash_deserialize_witness)
GitHub Check: fuzz (hashes_sha512)
GitHub Check: fuzz (dash_deserialize_block)
GitHub Check: fuzz (hashes_sha1)
GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
GitHub Check: fuzz (dash_deserialize_amount)
GitHub Check: fuzz (hashes_sha256)
GitHub Check: fuzz (hashes_json)
GitHub Check: fuzz (dash_outpoint_string)
GitHub Check: fuzz (dash_deser_net_msg)
GitHub Check: fuzz (dash_deserialize_script)
GitHub Check: RPC Tests (stable, true)
GitHub Check: Strict Warnings and Clippy Checks
GitHub Check: Core Components Tests
GitHub Check: SPV Components Tests

🔇 Additional comments (2)

key-wallet/src/gap_limit.rs (2)

16-20: Well-tested gap limit values.

The updated values (30 for internal and CoinJoin) align well with the extensive testing documented in the PR discussion, where restoration with a real iOS test wallet was verified to work correctly with these limits.

22-23: Doc comment correctly updated.

The doc comment for DEFAULT_SPECIAL_GAP_LIMIT has been appropriately corrected to describe special purpose keys (identity, provider keys), addressing the previous review feedback.

coderabbitai · 2025-10-07T17:59:54Z

key-wallet/src/gap_limit.rs

 /// Standard gap limit for external addresses (BIP44 recommendation)
-pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 20;
+pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 30;


⚠️ Potential issue | 🟡 Minor

Update the doc comment to clarify deviation from BIP44.

The doc comment states this follows the "BIP44 recommendation," but BIP44 actually recommends a gap limit of 20. The value of 30 was chosen based on testing with iOS/Android wallet compatibility (as discussed in the PR comments).

Consider updating the comment to reflect this:

-/// Standard gap limit for external addresses (BIP44 recommendation) +/// Standard gap limit for external addresses (increased from BIP44's recommended 20 for mobile wallet compatibility) pub const DEFAULT_EXTERNAL_GAP_LIMIT: u32 = 30;

🤖 Prompt for AI Agents

In key-wallet/src/gap_limit.rs around lines 13 to 14, the doc comment incorrectly states this value follows the "BIP44 recommendation" while BIP44 recommends a gap limit of 20; update the comment to explain that DEFAULT_EXTERNAL_GAP_LIMIT is set to 30 to support iOS/Android wallet compatibility based on testing and explicitly note it deviates from the BIP44 recommendation of 20.

HashEngineering · 2025-10-07T21:50:55Z

@QuantumExplorer I have made changes to gap limits. All are 30, except the special is set to 5. 30 is based on testing a coinjoin wallet and our test wallet from 2019.

Regarding filter sizes, I get your point. Though if a wallet uses CoinJoin, the number of keys involved increases by many orders of magnitude above a gap limit of 30 or 100. There are probably optimizations that can be done.

Also, the gap limits may need to be changed to be higher. Though we might consider having default settings for a new wallet vs a restores wallet that could have come from any other app. This can apply to the default derivation paths. Our test wallet has a transaction from 2019 on a BIP32 path. We discussed this before, where a new wallet doesn't need BIP32, but a restored wallet may need that path to find transactions.

QuantumExplorer · 2025-10-14T08:57:37Z

okay, sounds good.

HashEngineering added 2 commits October 3, 2025 16:05

fix: add a BIP32 path to Default wallet creation options

9f845aa

fix: increase gap limits to match Android and iOS defaults

0f0978e

coderabbitai bot reviewed Oct 3, 2025

View reviewed changes

key-wallet/src/gap_limit.rs Outdated Show resolved Hide resolved

key-wallet/src/wallet/helper.rs Show resolved Hide resolved

HashEngineering added 4 commits October 3, 2025 18:57

chore: fix special gap limit comment

cc8d49c

fix: add BIP32 account 0 to create_accounts_with_passphrase_from_options

bbf43a9

style: format to pass fmt

971be53

tests: update tests for adding BIP32 to Default wallet

7fc5df7

coderabbitai bot reviewed Oct 4, 2025

View reviewed changes

HashEngineering added 3 commits October 3, 2025 19:38

style: add missing ,

430da5b

chore: remove unused import

3cdd496

chore: fix comment in test

7bec305

QuantumExplorer requested changes Oct 4, 2025

View reviewed changes

fix: lower gap limits based on testing with one old wallet from 2019

a282394

coderabbitai bot reviewed Oct 7, 2025

View reviewed changes

style: fix format

c65c6e1

HashEngineering requested review from PastaPastaPasta and QuantumExplorer October 8, 2025 21:12

QuantumExplorer merged commit da5f9ec into dashpay:v0.40-dev Oct 14, 2025
34 of 35 checks passed

fix:adjust default paths and gap limits to match android/iOS behavior #156

fix:adjust default paths and gap limits to match android/iOS behavior #156

Uh oh!

Conversation

HashEngineering commented Oct 3, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QuantumExplorer commented Oct 4, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

HashEngineering commented Oct 7, 2025

Uh oh!

Uh oh!

QuantumExplorer commented Oct 14, 2025

Uh oh!

Reviewers

HashEngineering commented Oct 3, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 3, 2025 •

edited

Loading